fix: harden N-API boundary against JS-triggerable memory bugs#60
Open
nazarhussain wants to merge 6 commits into
Open
fix: harden N-API boundary against JS-triggerable memory bugs#60nazarhussain wants to merge 6 commits into
nazarhussain wants to merge 6 commits into
Conversation
napi sets the out word_count to the required count, which can exceed the caller's buffer; slicing by it read out of bounds for BigInts wider than the buffer (panic in safe builds, UB in ReleaseFast). getValueBigintWords returns error.Overflow instead. toI128 now range-checks the magnitude and handles -2^127, which previously tripped @intcast before negation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Status, ValueType, and TypedarrayType were exhaustive enums populated straight from napi out-params; a status or array type this binding doesn't know (e.g. Float16Array, statuses added in newer Node) was invalid-enum UB in ReleaseFast and a panic in safe builds. Make them non-exhaustive and map unknown values to errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Without new, V8 hands the global proxy as the receiver, so the generated constructor wrapped native state and a finalizer onto globalThis and type-tagged it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- wrapTaggedObject destroyed the native object on tagging failure while materializeClassInstance's errdefer destroyed it again (double free); ownership now stays with the caller on error - registerClass linked the entry before addEnvCleanupHook, so a hook failure freed the list head while still reachable (use-after-free) and leaked the ctor ref - module registration aborted via catch unreachable when throwError failed with an exception already pending None of these paths are reachable from the JS test harness (they need napi calls failing mid-sequence), hence no regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
expectType and expectTypedArrayOfType were called by every as*() narrowing method but never defined; Zig's lazy analysis hid it until a consumer instantiated one, which then failed to compile. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nazarhussain
marked this pull request as ready for review
July 23, 2026 07:09
Member
There was a problem hiding this comment.
safe builds panic (process abort / DoS); ReleaseFast — the usual production build — gets UB.
This is not true, we're using ReleaseSafe for zapi. I don't think we should use ReleaseSafe anywhere accept for bls (the rust bindings is compiled with -O3 which is == ReleaseFast)
| /// Read a BigInt's first u64 word via `getValueBigintWords` passing `null` for `sign_bit`. | ||
| /// | ||
| /// Throws if word_count > 1. | ||
| /// Throws `Overflow` if the BigInt needs more than one word. |
Member
There was a problem hiding this comment.
If this is the intended behaviour, maybe rename to bigIntSingleWord? bigIntFirstWord seems to imply there's more than one word and we're taking only the first
| } | ||
|
|
||
| /// Narrow an untyped value to a Uint8Array and return its length. | ||
| pub fn narrowToUint8ArrayLen(v: Value) !Number { |
Member
There was a problem hiding this comment.
do we need the Len suffix?
| } | ||
|
|
||
| /// On error the caller keeps ownership of `native_object`: the wrap is | ||
| /// detached so the finalizer cannot fire, but the object is not destroyed. |
Member
There was a problem hiding this comment.
i think maybe it's worth documenting why we don't want the finalizer to fire for now (because consumer cleans it up), this comment in itself isn't helpful w/o context imo
| bigint64 = c.napi_bigint64_array, | ||
| biguint64 = c.napi_biguint64_array, | ||
| // Non-exhaustive: engines already ship array types napi doesn't name yet | ||
| // (e.g. Float16Array); unknown values must be representable, not UB. |
Member
There was a problem hiding this comment.
question: i know the _ is to account for future upgrades adding new types, but do we necessarily want to silently accept and not UB?
Member
|
also, it appears some of BLS code use these codepaths, we could perhaps have regression tests aimed at what they used? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes JS-triggerable memory-safety bugs found in a security audit of
src/jsand the N-API layer.Fixes
getValueBigintWords): napi reports the required word count, which can exceed the caller's buffer; slicing by it read out of bounds. Now returnserror.Overflow.BigInt.toI128overflow: out-of-range magnitudes and the valid-2^127both tripped@intCast. Now range-checked.Status,ValueType,TypedarrayType): exhaustive enums from napi out-params were invalid-enum UB (e.g.Float16Array, newer statuses). Made non-exhaustive; unknown values map to errors.new: wrapped native state ontoglobalThis. Now throwsTypeError.materializeClassInstance, use-after-free inregisterClasslist linking,catch unreachablein module registration.expectType/expectTypedArrayOfType, which everyjs.Value.as*()method called but were never defined (compile error on use, hidden by lazy analysis).Safe builds panic (process abort / DoS);
ReleaseFast— the usual production build — gets UB.Tests
zig build test:napi/test:zapi,pnpm test:jsall greennewcalls, value narrowingexamples/targetshas one pre-existing, unrelated failure (musl detection on macOS)🤖 Generated with Claude Code